Fix inconsistent analysis on case-insensitive filesystems #3988
Conversation
clxmstaab
force-pushed
the
bug12972
branch
2 times, most recently
from
0bfa507 to
23a7403
Compare
|
This pull request has been marked as ready for review. |
| - name: "Install GNU Patch on macOS" # see https://github.com/cweagans/composer-patches/issues/326 | ||
| if: runner.os == 'macOS' | ||
| run: | | ||
| brew install gpatch | ||
| echo "/opt/homebrew/opt/gpatch/libexec/gnubin" >> $GITHUB_PATH | ||
|
|
There was a problem hiding this comment.
on macOS we need a GNU compatible patch command, as the apple-shipped patch command cannot apply our patch files:
Error: Cannot apply patch 0 (patches/cloudflare-ca.patch)!
In Patches.php line 331:
Cannot apply patch 0 (patches/cloudflare-ca.patch)!
| public function __construct(array $files = []) | ||
| { | ||
| $this->caseInsensitiveFilesystem = PHP_OS_FAMILY === 'Darwin'; | ||
| $this->setAnalysedFiles($files); | ||
| } | ||
|
|
||
| /** | ||
| * @param string[] $files | ||
| */ | ||
| public function setAnalysedFiles(array $files): void | ||
| { | ||
| if ($this->caseInsensitiveFilesystem) { | ||
| $files = array_map(static fn (string $file): string => strtolower($file), $files); | ||
| } | ||
| $this->analysedFiles = array_fill_keys($files, true); | ||
| } | ||
|
|
||
| public function isInAnalyzedFiles(string $file): bool | ||
| { | ||
| if ($this->caseInsensitiveFilesystem) { | ||
| $file = strtolower($file); | ||
| } | ||
|
|
||
| return isset($this->analysedFiles[$file]); | ||
| } |
There was a problem hiding this comment.
actual fix is here, everything else are adjustments to make sure this new logic is used all over the place
clxmstaab
force-pushed
the
bug12972
branch
3 times, most recently
from
72d0d20 to
d283dbe
Compare
Update e2e-tests.yml Update e2e-tests.yml Update e2e-tests.yml Update e2e-tests.yml Update e2e-tests.yml Update e2e-tests.yml Update e2e-tests.yml fix PathRoutingParser extract and re-use AnalysedFilesResolver use AnalysedFilesResolver more use AnalysedFilesResolver more simplify test also on windows added failling ubuntu test added feature detect simplify detect more assertions simplify Update AnalysedFilesResolver.php
| - script: | | ||
| cd e2e/bug-12972 | ||
| ../../bin/phpstan analyze | ||
| os: "windows-latest" |
There was a problem hiding this comment.
I don't get it. Why do you expect different outcomes on Linux vs. the rest?
There was a problem hiding this comment.
because linux does not have a case-insensitive filesystem.
There was a problem hiding this comment.
So you have a project that deliberately works only on case-insensitive filesystems?
There was a problem hiding this comment.
the test mimics what php-src will do at runtime.
it will happily accept a different-cased path on windows/macos but doesn't on linux (acutally depending on the filesystem case-sensitivy not the OS itself).
There was a problem hiding this comment.
I'd like if the e2e test mimiced how your project actually worked. This just looks like PHPStan fixes macOS but breaks Linux.
There was a problem hiding this comment.
ok, lets look into #4003 which mimics our project setup more closely
closes phpstan/phpstan#12972
since on macos filesystem paths are case-insensitive before this PR we ran into the following error:
-> PHPStan is using the simple-parser instead of the rich-parser as it does not handle the analyzed path case-insentive as macOS would do (same for windows)
see